Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for IP mediums, v2 #346

Closed
wants to merge 6 commits into from

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Jun 4, 2020

This is a new attempt at implementing #334, addressing the shortcomings found in the first version (#336) detailed in #336 (comment).

So far I'm quite pleased by how it turned out. The only big structural change in Interface is checking for Device.medium() in send and receive paths and adjusting the behavior accordingly. The rest of the changes are mostly boilerplate and updating the tests.

This includes the Linux tun work by @LucasZanella from Dirbaio#1, squashed and updated for the v2 changes.

Things that need fixing:

  • The prettyprinter is broken (it doesn't take medium into account, always interprets as Ethernet)

Possible future work:

  • Adding a medium_ip feature DONE
  • Renaming ethernet feature to medium_ethernet for consistency? DONE
  • Doing a PoC of instantiating an Interface with dynamic dispatch to the device, check how much improvement in binary size is there vs static dispatch.

@lattice0
Copy link
Contributor

lattice0 commented Jun 4, 2020

Very nice. Did a superficial inspection and it looks much better, with less code duplication. I was also bothered by having to duplicate the ethernet to tun interface.

Gonna test everything tomorrow, thanks!

@Dirbaio
Copy link
Member Author

Dirbaio commented Jun 4, 2020

I've pushed a new set of commits removing Split Packet into EthernetPacket and IpPacket.. Since we're no longer actually splitting EthernetInterface, it wasn't useful anymore, and it was adding unnecessary complexity.

@whitequark let me know what you think!

@Dirbaio
Copy link
Member Author

Dirbaio commented Jun 18, 2020

Rebased to latest master.

@whitequark I was wondering if you've had time to look at it. Is this new approach something you'd be interested in merging? any feedback?

@whitequark
Copy link
Contributor

I'll take a look soon.

@lattice0
Copy link
Contributor

I've been using this fork and it works :)

@whitequark
Copy link
Contributor

I'm going to review this over the weekend.

@lattice0
Copy link
Contributor

Has anyone tested this fork with a high transfer rate application?

I made a C++ interface for it based on this pull request: https://github.com/lucaszanella/smoltcp_cpp_interface and then I used it to send IP packets through an OpenVPN connection (that I modified to accept packets from memory rather than from tun/tap)

I modified the socket of an RTSP client to be a socket that sends things through this smol VPN tunnel that I created, but after some seconds (sometimes 1 second, sometimes 5 seconds), the client receives a packet that it shouldn't receive and stops working.

I'm debugging this problem for more than a week and couldn't find anything wrong on my C++ interface. I captured the RTSP/RTP packets of the camera using wireshark, and compared wth packets dumped from the smol stack. I wrote a python script that verifies packet per packet if the packet produced by smol stack matches with the packet produced by the camera. After some packets it finds one that was delivered by the stack that was not delivered by the camera. It's not a malformed packet, is just a different one.

If anyone has any idea on what might be happening I'd be happy to know!

@lattice0
Copy link
Contributor

Hi, nothing is wrong with these changes, I found my error. This pull request works perfectly, I already did a libcurl downloader and an RTSP client that uses this PR and it's working great!

@branchseer
Copy link

Hi @whitequark , is there any plan for merging this PR?

@whitequark
Copy link
Contributor

Hi @whitequark , is there any plan for merging this PR?

Sure, I'll review it when I have time.

Functions that only deal with IP packets take/return IpPacket's. IpPacket's
are wrapped into EthernetPacket's as late as possible.

This will later allow generalizing Interface to handle both Ethernet
and pure-IP mediums.
@Dirbaio
Copy link
Member Author

Dirbaio commented Sep 22, 2020

Rebased to latest master

@Dirbaio Dirbaio mentioned this pull request Sep 22, 2020
Dirbaio and others added 4 commits October 15, 2020 12:33
- Add `medium` in `DeviceCapabilities`.
- Rename EthernetInterface to Interface.
- Add support to Interface for both Ethernet and IP mediums. The medium to use is detected from `device.capabilities().medium`.
- Ethernet-only features are gated behind the "ethernet" feature, as before.
- IP features are always enabled for now.
@Dirbaio
Copy link
Member Author

Dirbaio commented Oct 21, 2020

Pushed new version:

  • Instead of a new medium() method, there's a new medium field in DeviceCapabilities. This is "more" backwards compatible, as it defaults to Ethernet. It also makes the trait cleaner IMO.
  • Added medium-ethernet and medium-ip features. At least one is required to build iface stuff.

@lattice0
Copy link
Contributor

I'm lost, where is the tun_interface.rs that I added on https://github.com/smoltcp-rs/smoltcp/tree/master/src/phy? I've been working with my fork for a long time and now wanted to use the new changes.

@Dirbaio
Copy link
Member Author

Dirbaio commented Mar 10, 2021

PR moved to #401 which includes your TunInterface work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants